*: use user keyspace's collation mode for DXF tasks by joechenrh · Pull Request #69677 · pingcap/tidb

joechenrh · 2026-07-06T03:51:13Z

What problem does this PR solve?

Issue Number: ref #69563

Problem Summary:

In next-gen deployments, DXF workers may run in a keyspace whose mysql.tidb.new_collation_enabled differs from the submitting user keyspace. If ADD INDEX or IMPORT INTO encodes user-table data with the worker-global setting, generated keys or restored row data can become inconsistent with the user table.

What changed and how does it work?

Store the submitting table snapshot's new-collation mode in ADD INDEX reorg metadata and IMPORT INTO plans.
Build DXF table, index, and codec helpers with that snapshot for affected encode/decode paths.
Fall back to the current process default for old task metadata that does not carry the snapshot.

Fix scope in this PR:

Supported:

DXF ADD INDEX and IMPORT INTO key/data encoding for non-partitioned tables.
Ordinary string transformation expressions whose result is encoded into table/index keys. This is covered for LOWER(), UPPER(), CONCAT(), and SUBSTR() in generated columns, expression indexes, and IMPORT INTO column assignments.

Out of scope / not validated in this PR:

Partitioned tables.
Partial indexes.
Expressions whose result depends on collation-sensitive comparison, search, ordering, or weight semantics, including =, <=>, !=, <, <=, >, >=, IN, LIKE/ILIKE/REGEXP, IF/CASE branches using those predicates, STRCMP(), FIELD(), LOCATE()/INSTR(), GREATEST()/LEAST(), and WEIGHT_STRING().

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Manual test setup:

Local next-gen CSE cluster.
User keyspace: mysql.tidb.new_collation_enabled = False.
System keyspace: mysql.tidb.new_collation_enabled = True.
Compared master local build with this PR local build.
For every test below, validation includes ADMIN CHECK TABLE, forced index reads vs primary reads, and post-load/post-backfill DML (INSERT, UPDATE, DELETE) followed by another ADMIN CHECK TABLE and forced index reads.

IMPORT INTO:

Case	Master result	This PR result
Clustered VARCHAR PK + secondary VARCHAR index	Fails: secondary covering read decodes empty strings; `ADMIN CHECK TABLE` fails.	Passes.
Clustered VARCHAR PK + secondary INT index	Fails: secondary read decodes empty VARCHAR handle values.	Passes.
Composite clustered PK with VARCHAR part + secondary INT index	Fails: secondary read decodes empty `id1` values.	Passes.
Composite clustered INT PK + secondary VARCHAR index	Fails: secondary read decodes the VARCHAR index value incorrectly.	Passes.
Composite CHAR PK + secondary VARCHAR index	Fails: secondary read reports missing decoded columns or table/index inconsistency.	Passes.
Clustered VARCHAR PK + prefix secondary VARCHAR index	Fails: IndexLookUp through `idx_fk_prefix` reports `index-count:3 != record-count:0`.	Passes.
Clustered VARCHAR PK + secondary VARCHAR index + extra payload	Fails: primary/table reads succeed, but secondary IndexLookUp reports `index-count:1 != record-count:0`.	Passes.
Generated columns with string transformations	Fails: `ADMIN CHECK TABLE` reports data inconsistency; forced reads through all four generated-column indexes report `index-count:3 != record-count:0`.	Passes, including DML after import.
Assignment expressions with string transformations	Fails: `ADMIN CHECK TABLE` reports data inconsistency; forced reads through all four assignment indexes report `index-count:3 != record-count:0`.	Passes, including DML after import.
Controls without mismatched string collation in encoded keys	Passes.	Passes.

IMPORT INTO SQL details

Clustered VARCHAR PK + secondary VARCHAR index

Schema:

CREATE TABLE trigger_varchar_pk_varchar_idx (
  id varchar(32),
  fk varchar(32),
  PRIMARY KEY (id),
  KEY idx_fk (fk)
);

Import mapping:

IMPORT INTO trigger_varchar_pk_varchar_idx(@1,id,fk);

Clustered VARCHAR PK + secondary INT index

Schema:

CREATE TABLE trigger_varchar_pk_int_idx (
  id varchar(32),
  fk int,
  PRIMARY KEY (id),
  KEY idx_fk (fk)
);

Import mapping:

IMPORT INTO trigger_varchar_pk_int_idx(fk,id,@3);

Composite clustered PK with VARCHAR part + secondary INT index

Schema:

CREATE TABLE trigger_composite_varchar_int_pk_int_idx (
  id1 varchar(32),
  id2 int,
  fk int,
  PRIMARY KEY (id1, id2) CLUSTERED,
  KEY idx_fk (fk)
);

Import mapping:

IMPORT INTO trigger_composite_varchar_int_pk_int_idx(id2,fk,id1);

Composite clustered INT PK + secondary VARCHAR index

Schema:

CREATE TABLE trigger_composite_int_int_pk_varchar_idx (
  id1 int,
  id2 int,
  fk varchar(32),
  PRIMARY KEY (id1, id2) CLUSTERED,
  KEY idx_fk (fk)
);

Import mapping:

IMPORT INTO trigger_composite_int_int_pk_varchar_idx(id1,id2,fk);

Composite CHAR PK + secondary VARCHAR index

Schema:

CREATE TABLE trigger_composite_char_char_pk_varchar_idx (
  id1 char(32),
  id2 char(32),
  fk varchar(32),
  PRIMARY KEY (id1, id2) CLUSTERED,
  KEY idx_fk (fk)
);

Import mapping:

IMPORT INTO trigger_composite_char_char_pk_varchar_idx(fk,id1,id2);

Clustered VARCHAR PK + prefix secondary VARCHAR index

Schema:

CREATE TABLE trigger_varchar_pk_prefix_varchar_idx (
  id varchar(32),
  fk varchar(32),
  PRIMARY KEY (id) CLUSTERED,
  KEY idx_fk_prefix (fk(2))
);

Import mapping:

IMPORT INTO trigger_varchar_pk_prefix_varchar_idx(@1,id,fk);

Clustered VARCHAR PK + secondary VARCHAR index + extra payload

Schema:

CREATE TABLE trigger_record_ok_index_bad_extra_payload (
  id varchar(32),
  fk varchar(32),
  payload varchar(32) DEFAULT 'payload',
  PRIMARY KEY (id) CLUSTERED,
  KEY idx_fk (fk)
);

Import mapping:

IMPORT INTO trigger_record_ok_index_bad_extra_payload(@1,id,fk);

Generated columns with string transformations

Schema:

CREATE TABLE t_import_generated (
  id varchar(32),
  raw varchar(32),
  g_lower varchar(32) GENERATED ALWAYS AS (lower(raw)) STORED,
  g_upper varchar(32) GENERATED ALWAYS AS (upper(raw)) STORED,
  g_concat varchar(80) GENERATED ALWAYS AS (concat(id, ':', raw)) STORED,
  g_substr varchar(32) GENERATED ALWAYS AS (substr(raw, 1, 2)) STORED,
  PRIMARY KEY (id) CLUSTERED,
  KEY idx_lower (g_lower),
  KEY idx_upper (g_upper),
  KEY idx_concat (g_concat),
  KEY idx_substr (g_substr)
);

Import mapping:

IMPORT INTO t_import_generated(@1,id,raw);

Assignment expressions with string transformations

Schema:

CREATE TABLE t_import_assignment (
  id varchar(32),
  raw varchar(32),
  a_lower varchar(32),
  a_upper varchar(32),
  a_concat varchar(80),
  a_substr varchar(32),
  PRIMARY KEY (id) CLUSTERED,
  KEY idx_lower (a_lower),
  KEY idx_upper (a_upper),
  KEY idx_concat (a_concat),
  KEY idx_substr (a_substr)
);

Import mapping / SET:

IMPORT INTO t_import_assignment(@1,@2,@3)
SET id=@2,
    raw=@3,
    a_lower=lower(@3),
    a_upper=upper(@3),
    a_concat=concat(@2, ':', @3),
    a_substr=substr(@3, 1, 2);

Controls without mismatched string collation in encoded keys

Cases:

ok_int_pk_varchar_idx
ok_varchar_pk_no_secondary
ok_nonclustered_varchar_pk_varchar_idx
ok_char_pk_char_idx
ok_composite_int_int_pk_int_idx
ok_composite_varbinary_int_pk_int_idx
ok_composite_char_char_pk_int_idx

ADD INDEX:

Case	Master result	This PR result
Clustered VARCHAR PK + new secondary VARCHAR index	Fails: `ADMIN CHECK TABLE` or secondary-index reads show table/index inconsistency.	Passes.
Composite clustered PK with VARCHAR part + new secondary INT index	Fails: `ADMIN CHECK TABLE` or secondary-index reads show table/index inconsistency.	Passes.
Generated columns with string transformations	Fails: `ADMIN CHECK TABLE` reports data inconsistency; forced reads through all four generated-column indexes report table/index count mismatch.	Passes, including DML after ADD INDEX.
Expression indexes with string transformations	Fails: `ADMIN CHECK TABLE` reports data inconsistency; forced reads through all four expression indexes report `index-count:3 != record-count:0`.	Passes, including DML after ADD INDEX.

ADD INDEX SQL details

Clustered VARCHAR PK + new secondary VARCHAR index

Schema / DML:

CREATE TABLE t_varchar_pk (
  id varchar(32),
  fk varchar(32),
  PRIMARY KEY (id) CLUSTERED
);
INSERT INTO t_varchar_pk VALUES ('aaa', 'abc'), ('bbb', 'bbc'), ('ccc', 'cbc');

ADD INDEX DDL:

ALTER TABLE t_varchar_pk ADD INDEX idx_fk(fk);

Composite clustered PK with VARCHAR part + new secondary INT index

Schema / DML:

CREATE TABLE t_composite_varchar_pk (
  id1 varchar(32),
  id2 int,
  fk int,
  PRIMARY KEY (id1, id2) CLUSTERED
);
INSERT INTO t_composite_varchar_pk VALUES ('ax', 1, 10), ('by', 2, 20), ('cz', 3, 30);

ADD INDEX DDL:

ALTER TABLE t_composite_varchar_pk ADD INDEX idx_fk(fk);

Generated columns with string transformations

Schema / DML:

CREATE TABLE t_add_generated_column_index (
  id varchar(32),
  raw varchar(32),
  g_lower varchar(32) GENERATED ALWAYS AS (lower(raw)) VIRTUAL,
  g_upper varchar(32) GENERATED ALWAYS AS (upper(raw)) VIRTUAL,
  g_concat varchar(80) GENERATED ALWAYS AS (concat(id, ':', raw)) VIRTUAL,
  g_substr varchar(32) GENERATED ALWAYS AS (substr(raw, 1, 2)) VIRTUAL,
  PRIMARY KEY (id) CLUSTERED
);
INSERT INTO t_add_generated_column_index(id, raw) VALUES ('aaa', 'abc'), ('bbb', 'bbc'), ('ccc', 'cbc');

ADD INDEX DDL:

ALTER TABLE t_add_generated_column_index ADD INDEX idx_g_lower(g_lower);
ALTER TABLE t_add_generated_column_index ADD INDEX idx_g_upper(g_upper);
ALTER TABLE t_add_generated_column_index ADD INDEX idx_g_concat(g_concat);
ALTER TABLE t_add_generated_column_index ADD INDEX idx_g_substr(g_substr);

Expression indexes with string transformations

Schema / DML:

CREATE TABLE t_add_expression_index (
  id varchar(32),
  raw varchar(32),
  PRIMARY KEY (id) CLUSTERED
);
INSERT INTO t_add_expression_index VALUES ('aaa', 'abc'), ('bbb', 'bbc'), ('ccc', 'cbc');

ADD INDEX DDL:

ALTER TABLE t_add_expression_index ADD INDEX idx_lower ((lower(raw)));
ALTER TABLE t_add_expression_index ADD INDEX idx_upper ((upper(raw)));
ALTER TABLE t_add_expression_index ADD INDEX idx_concat ((concat(id, ':', raw)));
ALTER TABLE t_add_expression_index ADD INDEX idx_substr ((substr(raw, 1, 2)));

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Fix possible index and row data inconsistency when DXF ADD INDEX or IMPORT INTO encodes string data with a new-collation mode different from the submitting keyspace.

coderabbitai · 2026-07-06T03:51:40Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR threads UseNewCollate through table contracts, metadata, DDL reorg/backfill, expression building, and import/lightning table construction. It also updates row/handle decoding call sites to use table-derived collation behavior.

Changes

UseNewCollate Plumbing

Layer / File(s)	Summary
Table contract and collate-aware construction `pkg/table/table.go`, `pkg/meta/model/reorg.go`, `pkg/executor/importer/import.go`, `pkg/infoschema/tables.go`, `pkg/table/tables/tables.go`, `pkg/table/tables/partition.go`, `pkg/infoschema/BUILD.bazel`	Adds `UseNewCollate` to table-facing contracts, stores it on DDL/import metadata, and refactors table construction and partition setup around collate-aware table common initialization.
Row and restored-data decoding `pkg/table/tables/tables.go`, `lightning/pkg/errormanager/errormanager.go`, `pkg/dxf/importinto/conflictedkv/handler.go`, `pkg/executor/batch_checker.go`, `pkg/executor/admin.go`, `pkg/lightning/backend/kv/kv2sql.go`, `pkg/table/tables/mutation_checker_test.go`, `pkg/ddl/index.go`, `pkg/ddl/index_cop.go`	Changes raw-row decoding and restored-handle helpers to accept `table.Table`, then updates callers to pass tables instead of meta objects or explicit collation arguments.
Expression collation context and string builtins `pkg/expression/collation_context.go`, `pkg/expression/expression.go`, `pkg/expression/builtin.go`, `pkg/expression/builtin_compare.go`, `pkg/expression/builtin_compare_vec.go`, `pkg/expression/builtin_compare_vec_generated.go`, `pkg/expression/builtin_other.go`, `pkg/expression/builtin_other_vec_generated.go`, `pkg/expression/builtin_string.go`, `pkg/expression/builtin_string_vec.go`, `pkg/expression/generator/compare_vec.go`, `pkg/expression/generator/other_vec.go`, `pkg/expression/util.go`, `pkg/planner/core/expression_rewriter.go`, `pkg/planner/core/expression_test.go`	Adds build-context collation plumbing, refactors string comparison and `IN`/`LOCATE`/`WEIGHT_STRING` builtins to use collator-backed helpers, and updates generated compare templates and tests.
DDL reorg, backfill, and handle encoding `pkg/ddl/executor.go`, `pkg/ddl/export_test.go`, `pkg/ddl/backfilling_txn_executor.go`, `pkg/ddl/copr/copr_ctx.go`, `pkg/ddl/copr/copr_ctx_test.go`, `pkg/ddl/backfilling_dist_scheduler.go`, `pkg/ddl/backfilling_dist_executor.go`, `pkg/ddl/backfilling_operators.go`, `pkg/ddl/job_scheduler.go`, `pkg/ddl/reorg.go`	Threads `UseNewCollate` through DDL reorg metadata, coprocessor context construction, backfill table lookup, handle encoding, and reorg scanning, and removes the older transaction-based table lookup helper.
Import and Lightning table wiring `pkg/executor/importer/import.go`, `pkg/executor/importer/sampler.go`, `pkg/executor/importer/table_import.go`, `pkg/dxf/importinto/planner.go`, `pkg/dxf/importinto/task_executor.go`, `pkg/dxf/importinto/conflictedkv/handler.go`, `pkg/executor/importer/chunk_process_testkit_test.go`, `pkg/lightning/backend/kv/base.go`, `pkg/lightning/backend/kv/kv2sql.go`, `pkg/lightning/backend/kv/sql2kv.go`, `pkg/dxf/importinto/BUILD.bazel`, `pkg/executor/importer/BUILD.bazel`	Switches import and Lightning table construction to collate-aware table factories, propagates the plan-level flag, updates generated-column expression setup, and adjusts build dependencies and test wiring.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DDLExecutor
  participant DDLReorgMeta
  participant backfillDistExecutor
  participant copr
  participant tables
  DDLExecutor->>DDLReorgMeta: set UseNewCollate
  backfillDistExecutor->>DDLReorgMeta: read UseNewCollate default
  backfillDistExecutor->>copr: NewCopContext(..., useNewCollate)
  copr->>tables: ColumnInfos2ColumnsAndNamesWithCollate(...)
  backfillDistExecutor->>tables: TableFromMetaWithCollate(...)
  tables-->>backfillDistExecutor: collate-aware table

sequenceDiagram
  participant LoadDataController
  participant Plan
  participant tables
  participant expression
  LoadDataController->>Plan: GetUseNewCollateOrDefault(...)
  LoadDataController->>tables: TableFromMetaWithCollate(...)
  LoadDataController->>expression: BuildSimpleExpr(..., WithUseNewCollate(...))
  tables-->>LoadDataController: collate-aware table
  expression-->>LoadDataController: generated-column expression

Possibly related PRs

pingcap/tidb#69566: Aligns with the same table/codec collation plumbing across encoding and decoding paths.

Suggested reviewers: wjhuang2016, qw4990, windtalker

Poem

A rabbit hops through keys and rows,
With collate mode in steady flows.
Tables hum and handles gleam,
New flags stitch the whole big stream,
Hop-hop — the encode paths now glow. 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately summarizes the PR’s main change: using the user keyspace’s collation mode for DXF tasks.
Description check	✅ Passed	The description matches the template and includes problem summary, changes, tests, side effects, and release note.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

pkg/ddl/index.go (1)
2700-2716: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Derive useNewCollate from copCtx inside writeChunk.

writeChunk already has c := copCtx.GetBase(), which now carries UseNewCollate. Taking a separate boolean makes it possible for callers to encode handles/restored data with a different collation than the cop context used for expression metadata.
Proposed direction
 func writeChunk(
 	ctx context.Context,
 	writers []ingest.Writer,
 	indexes []table.Index,
 	indexConditionCheckers []func(row chunk.Row) (bool, error),
 	copCtx copr.CopContext,
 	loc *time.Location,
 	errCtx errctx.Context,
 	writeStmtBufs *variable.WriteStmtBufs,
 	copChunk *chunk.Chunk,
 	tblInfo *model.TableInfo,
-	useNewCollate bool,
 ) (rowCnt int, bytes int, err error) {
 	iter := chunk.NewIterator4Chunk(copChunk)
 	c := copCtx.GetBase()
+	useNewCollate := c.UseNewCollate
Also applies to: 2736-2779
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/ddl/index.go` around lines 2700 - 2716, `writeChunk` should not accept a
separate `useNewCollate` flag; derive the collation mode from `copCtx.GetBase()`
inside `writeChunk` so encoding stays consistent with the cop context’s
expression metadata. Update `writeChunk` and its call sites to use the
`UseNewCollate` value from `copCtx` (through the existing `c :=
copCtx.GetBase()` path) and remove the extra boolean parameter to prevent
callers from passing mismatched collation state.
pkg/ddl/backfilling_operators.go (1)
114-121: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Use the persisted reorg collation for index encoding too. tbl.UseNewCollate() comes from the live table object, while NewReorgCopContext uses the reorg snapshot; if those differ on a resumed/background job, scan metadata and index KV encoding can drift apart. Reuse copCtx.GetBase().UseNewCollate here and at the other callsite.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/ddl/backfilling_operators.go` around lines 114 - 121, The index encoding
path is still using the live table collation via tbl.UseNewCollate(), which can
diverge from the reorg snapshot used by NewReorgCopContext on resumed jobs.
Update the index creation logic in backfilling_operators.go to reuse
copCtx.GetBase().UseNewCollate for tables.NewIndexWithCollate, and apply the
same change at the other callsite so scan metadata and KV encoding stay
consistent.
pkg/ddl/export_test.go (1)
77-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use the CopContext's captured collation instead of re-querying the process default.

copCtx already carries the collation it was built with (CopContextBase.UseNewCollate, set by NewCopContext/NewReorgCopContext), obtained here via c := copCtx.GetBase(). Calling collate.NewCollationEnabled() at line 84 ignores that captured value and re-reads the worker's current global setting instead. This defeats the purpose of this exact PR stack (avoiding worker-global lookups in favor of a captured/task-level value) and means this test helper cannot correctly exercise scenarios where the CopContext's collation differs from the process default.
🐛 Proposed fix
-	handle, err := ddl.BuildHandle(collate.NewCollationEnabled(), handleData, c.TableInfo, c.PrimaryKeyInfo, time.Local, errctx.StrictNoWarningContext)
+	handle, err := ddl.BuildHandle(c.UseNewCollate, handleData, c.TableInfo, c.PrimaryKeyInfo, time.Local, errctx.StrictNoWarningContext)
If collate becomes unused elsewhere in this file, remove the now-unnecessary import at line 36.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/ddl/export_test.go` around lines 77 - 86,
`ConvertRowToHandleAndIndexDatum` is ignoring the `CopContext`’s captured
collation and re-reading the process default via
`collate.NewCollationEnabled()`. Use the collation stored on `c :=
copCtx.GetBase()` when calling `ddl.BuildHandle` so this helper follows the
task-level setting from `NewCopContext`/`NewReorgCopContext`. If `collate` is no
longer needed in `export_test.go`, remove that import as well.

🧹 Nitpick comments (1)

pkg/ddl/copr/copr_ctx_test.go (1)
109-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the useNewCollate=true constructor path.

This only adapts the existing test with false; a regression that drops the new flag in NewCopContextBase would still pass. Add a small true-case assertion that base.UseNewCollate matches the argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/ddl/copr/copr_ctx_test.go` around lines 109 - 120, The current test only
exercises the NewCopContextSingleIndex path with useNewCollate set to false, so
it won’t catch regressions in the true path. Update the copr_ctx_test coverage
around NewCopContextSingleIndex and GetBase by adding a small assertion for the
useNewCollate=true constructor case and verify base.UseNewCollate matches the
argument, alongside the existing TableInfo check.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/ddl/backfilling_operators.go`:
- Around line 917-918: The writeChunk call is reading collation mode from w.tbl
again instead of using the fixed collation captured in w.copCtx. Update the call
site in backfilling_operators.go so writeChunk receives
w.copCtx.GetBase().UseNewCollate rather than w.tbl.UseNewCollate, keeping row
decoding, handle construction, and index writes aligned with the same snapshot.
Use the existing writeChunk invocation and w.copCtx reference as the location to
make this change.

---

Outside diff comments:
In `@pkg/ddl/backfilling_operators.go`:
- Around line 114-121: The index encoding path is still using the live table
collation via tbl.UseNewCollate(), which can diverge from the reorg snapshot
used by NewReorgCopContext on resumed jobs. Update the index creation logic in
backfilling_operators.go to reuse copCtx.GetBase().UseNewCollate for
tables.NewIndexWithCollate, and apply the same change at the other callsite so
scan metadata and KV encoding stay consistent.

In `@pkg/ddl/export_test.go`:
- Around line 77-86: `ConvertRowToHandleAndIndexDatum` is ignoring the
`CopContext`’s captured collation and re-reading the process default via
`collate.NewCollationEnabled()`. Use the collation stored on `c :=
copCtx.GetBase()` when calling `ddl.BuildHandle` so this helper follows the
task-level setting from `NewCopContext`/`NewReorgCopContext`. If `collate` is no
longer needed in `export_test.go`, remove that import as well.

In `@pkg/ddl/index.go`:
- Around line 2700-2716: `writeChunk` should not accept a separate
`useNewCollate` flag; derive the collation mode from `copCtx.GetBase()` inside
`writeChunk` so encoding stays consistent with the cop context’s expression
metadata. Update `writeChunk` and its call sites to use the `UseNewCollate`
value from `copCtx` (through the existing `c := copCtx.GetBase()` path) and
remove the extra boolean parameter to prevent callers from passing mismatched
collation state.

---

Nitpick comments:
In `@pkg/ddl/copr/copr_ctx_test.go`:
- Around line 109-120: The current test only exercises the
NewCopContextSingleIndex path with useNewCollate set to false, so it won’t catch
regressions in the true path. Update the copr_ctx_test coverage around
NewCopContextSingleIndex and GetBase by adding a small assertion for the
useNewCollate=true constructor case and verify base.UseNewCollate matches the
argument, alongside the existing TableInfo check.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e82e626e-b969-4548-a71f-cd1cb089b9a0

📥 Commits

Reviewing files that changed from the base of the PR and between ab1e197 and ef07673.

📒 Files selected for processing (32)

lightning/pkg/errormanager/errormanager.go
pkg/ddl/backfilling_dist_executor.go
pkg/ddl/backfilling_dist_scheduler.go
pkg/ddl/backfilling_operators.go
pkg/ddl/backfilling_txn_executor.go
pkg/ddl/copr/copr_ctx.go
pkg/ddl/copr/copr_ctx_test.go
pkg/ddl/executor.go
pkg/ddl/export_test.go
pkg/ddl/index.go
pkg/ddl/index_cop.go
pkg/ddl/job_scheduler.go
pkg/ddl/reorg.go
pkg/dxf/importinto/conflictedkv/handler.go
pkg/dxf/importinto/planner.go
pkg/dxf/importinto/task_executor.go
pkg/executor/admin.go
pkg/executor/batch_checker.go
pkg/executor/importer/import.go
pkg/executor/importer/sampler.go
pkg/executor/importer/table_import.go
pkg/expression/expression.go
pkg/infoschema/tables.go
pkg/lightning/backend/kv/base.go
pkg/lightning/backend/kv/kv2sql.go
pkg/lightning/backend/kv/sql2kv.go
pkg/meta/model/reorg.go
pkg/planner/core/expression_rewriter.go
pkg/table/table.go
pkg/table/tables/mutation_checker_test.go
pkg/table/tables/partition.go
pkg/table/tables/tables.go

💤 Files with no reviewable changes (1)

pkg/ddl/job_scheduler.go

Co-authored-by: D3Hunter <jujj603@gmail.com>

D3Hunter · 2026-07-06T10:13:10Z

/retest

joechenrh · 2026-07-06T10:46:22Z

/retest

ti-chi-bot · 2026-07-06T11:24:57Z

[LGTM Timeline notifier]

Timeline:

2026-07-06 10:21:30.670744478 +0000 UTC m=+17876.706839524: ☑️ agreed by D3Hunter.
2026-07-06 11:24:55.768177002 +0000 UTC m=+21681.804272068: ☑️ agreed by GMHDBJD.

ingress-bot

This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.

Summary

Total findings: 8
Inline comments: 3
Summary-only findings (no inline anchor): 5

Findings (highest risk first)

⚠️ [Major] (3)

CopContextBase.UseNewCollate is a dead field that looks authoritative but is never read (pkg/ddl/copr/copr_ctx.go:49, pkg/ddl/copr/copr_ctx.go:150, pkg/ddl/backfilling_operators.go:918)
Mixed-version rolling upgrade can produce inconsistent collation encoding for cross-keyspace add-index/import (pkg/ddl/backfilling_dist_scheduler.go:205, pkg/executor/importer/table_import.go:94, pkg/meta/model/reorg.go:98, pkg/executor/importer/import.go:338)
Rolling upgrade: in-flight reorg/import jobs with nil UseNewCollate fall back to the executor process default, risking split collation encoding (pkg/ddl/backfilling_dist_scheduler.go:206, pkg/meta/model/reorg.go:98, pkg/executor/importer/import.go:338, pkg/executor/importer/table_import.go:94)

🟡 [Minor] (3)

Error message still names the old tables.TableFromMeta function after the call site switched to TableFromMetaWithCollate (pkg/executor/importer/table_import.go:98, pkg/executor/importer/sampler.go:341)
Optional-collation-flag field/getter/setter duplicated verbatim across two packages (pkg/executor/importer/import.go:343, pkg/executor/importer/import.go:360, pkg/executor/importer/import.go:369, pkg/meta/model/reorg.go:103, pkg/meta/model/reorg.go:162, pkg/meta/model/reorg.go:171)
New useNewCollate flag is positioned inconsistently across sibling function signatures (pkg/table/tables/tables.go:173, pkg/table/tables/tables.go:248, pkg/ddl/index_cop.go:259, pkg/ddl/index_cop.go:148, pkg/ddl/index.go:2700, pkg/ddl/copr/copr_ctx.go:78, pkg/ddl/reorg.go:850)

ℹ️ [Info] (1)

NewCopContextBase doc comment wasn't updated to mention the new collation parameter, unlike its three sibling constructors (pkg/ddl/copr/copr_ctx.go:76)

🧹 [Nit] (1)

Identical admin-check-and-verify test helper duplicated across two realtikvtest packages (tests/realtikvtest/addindextest1/cross_ks_test.go:331, tests/realtikvtest/importintotest3/cross_ks_test.go:608)

Unanchored findings

⚠️ [Major] (3)

CopContextBase.UseNewCollate is a dead field that looks authoritative but is never read
- Request: Either make writeChunk/getRestoreData/related call sites consume copCtx.GetBase().UseNewCollate as the single source of truth, or drop the field from CopContextBase and its assignment in NewCopContextBase since it currently has no consumer.
Mixed-version rolling upgrade can produce inconsistent collation encoding for cross-keyspace add-index/import
- Request: Confirm whether DXF prevents pre-PR executors from running these subtasks during a rolling upgrade; if it does not, gate the differing-collation encoding until full upgrade (or document that cross-keyspace add-index/import-into on such keyspaces must not run mid-upgrade) and add a mixed-version validation for the encoding path.
Rolling upgrade: in-flight reorg/import jobs with nil UseNewCollate fall back to the executor process default, risking split collation encoding
- Request: Confirm whether DXF prevents pre-PR executors/in-flight jobs from resuming these subtasks during a rolling upgrade; if not, gate or reject the differing-collation encoding until the job's collation mode is known, and add a mixed-version/old-metadata test that resumes a nil-UseNewCollate job and asserts index consistency.

🟡 [Minor] (2)

Error message still names the old tables.TableFromMeta function after the call site switched to TableFromMetaWithCollate
- Request: Update both error strings to name tables.TableFromMetaWithCollate (or drop the function name and just say "failed to build table from meta") so the message matches the code that actually runs.
Optional-collation-flag field/getter/setter duplicated verbatim across two packages
- Request: Extract the pointer-bool-with-default pattern into one small shared helper type (for example in a common metadata/compat util package) and have both Plan and DDLReorgMeta embed or delegate to it, so the fallback semantics only exist in one place.

ingress-bot · 2026-07-06T11:46:55Z

-	t.recordPrefix = tablecodec.GenTableRecordPrefix(physicalTableID)
-	t.indexPrefix = tablecodec.GenTableIndexPrefix(physicalTableID)
-	t.encoder = codec.NewEncoder(collate.NewCollationEnabled())
+func newTableCommon(tblInfo *model.TableInfo, physicalTableID int64, cols []*table.Column, allocs autoid.Allocators, constraints []*table.Constraint, useNewCollate bool) TableCommon {


🟡 [Minor] New useNewCollate flag is positioned inconsistently across sibling function signatures

Impact
This PR threads a new useNewCollate bool flag through a large number of signatures, but its position in the parameter list flips depending on the function: TableFromMetaWithCollate and BuildHandle/getRestoreData put it first, while newTableCommon (called directly by TableFromMetaWithCollate two hunks later in the same file) and writeChunk/NewCopContextBase put it last.
With no single convention to copy, the next contributor adding a related flag (or wiring a new call site) has to re-derive the ordering for each function instead of pattern-matching a sibling, which is exactly the kind of drift risk that surfaces as a misplaced argument in a future refactor of these encoding-path helpers.

Scope

pkg/table/tables/tables.go:173 — TableFromMetaWithCollate

pkg/table/tables/tables.go:248 — newTableCommon

pkg/ddl/index_cop.go:259 — BuildHandle

pkg/ddl/index_cop.go:148 — getRestoreData

pkg/ddl/index.go:2700 — writeChunk

pkg/ddl/copr/copr_ctx.go:78 — NewCopContextBase

pkg/ddl/reorg.go:850 — buildCommonHandleFromChunkRow

Evidence
TableFromMetaWithCollate(useNewCollate bool, allocs autoid.Allocators, tblInfo *model.TableInfo) puts the flag first and immediately calls newTableCommon(tblInfo, physicalTableID, cols, allocs, constraints, useNewCollate), which puts the same flag last; the same split repeats between BuildHandle/getRestoreData (flag first) and writeChunk/NewCopContextBase (flag last).

Change request
Pick one convention for this new flag (trailing is the more common Go idiom here, matching writeChunk/NewCopContextBase) and apply it uniformly across TableFromMetaWithCollate, BuildHandle, getRestoreData, and buildCommonHandleFromChunkRow.

ingress-bot · 2026-07-06T11:46:55Z

 	tblInfo *model.TableInfo,
 	idxCols []*model.IndexColumn,
 	requestSource string,
+	useNewCollate bool,


ℹ️ [Info] NewCopContextBase doc comment wasn't updated to mention the new collation parameter, unlike its three sibling constructors

NewCopContext, NewCopContextSingleIndex, and NewCopContextMultiIndex all got their doc comments updated to say "creates a ... with a fixed collation mode", but NewCopContextBase — the function that actually receives and stores useNewCollate into the struct — kept its original one-line doc with no mention of the new parameter.

ingress-bot · 2026-07-06T11:46:55Z

+	}
+}
+
+func checkImportTableAndIndexes(tk *testkit.TestKit, tableName string, indexes []string, expectedCount string) {


🧹 [Nit] Identical admin-check-and-verify test helper duplicated across two realtikvtest packages

checkTableAndIndexes (addindextest1) and checkImportTableAndIndexes (importintotest3), both newly added by this PR, run the exact same three-step verification (admin check, row count, per-index force-index count) with only the function name differing.

windtalker

expression part lgtm

ti-chi-bot · 2026-07-06T12:23:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, GMHDBJD, windtalker

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [D3Hunter,GMHDBJD,windtalker]
~~lightning/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/ddl/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/dxf/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/executor/importer/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/expression/OWNERS~~ [windtalker]
~~pkg/infoschema/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/lightning/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/meta/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/table/OWNERS~~ [GMHDBJD]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

joechenrh · 2026-07-06T14:21:37Z

/test idc-jenkins-ci-tidb/check_dev_2

ti-chi-bot · 2026-07-06T14:21:42Z

@joechenrh: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build

/test check-dev

/test check-dev2

/test mysql-test

/test pull-br-integration-test

/test pull-build-next-gen

/test pull-integration-e2e-test

/test pull-integration-realcluster-test-next-gen

/test pull-lightning-integration-test

/test pull-mysql-client-test

/test pull-mysql-client-test-next-gen

/test pull-unit-test-ddlv1

/test pull-unit-test-next-gen

/test unit-test

The following commands are available to trigger optional jobs:

/test canary-long-unit-test

/test canary-unit-test

/test pull-br-integration-test-next-gen

/test pull-check-deps

/test pull-common-test

/test pull-e2e-test

/test pull-error-log-review

/test pull-integration-common-test

/test pull-integration-copr-test

/test pull-integration-ddl-test

/test pull-integration-ddl-test-next-gen

/test pull-integration-e2e-test-next-gen

/test pull-integration-jdbc-test

/test pull-integration-mysql-test

/test pull-integration-nodejs-test

/test pull-integration-python-orm-test

/test pull-mysql-test-next-gen

/test pull-sqllogic-test

/test pull-tiflash-integration-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build

pingcap/tidb/ghpr_check

pingcap/tidb/ghpr_check2

pingcap/tidb/ghpr_mysql_test

pingcap/tidb/ghpr_unit_test

pingcap/tidb/pull_br_integration_test

pingcap/tidb/pull_build_next_gen

pingcap/tidb/pull_integration_e2e_test

pingcap/tidb/pull_integration_realcluster_test_next_gen

pingcap/tidb/pull_lightning_integration_test

pingcap/tidb/pull_mysql_client_test

pingcap/tidb/pull_mysql_client_test_next_gen

pingcap/tidb/pull_unit_test_ddlv1

pingcap/tidb/pull_unit_test_next_gen

pull-check-deps

pull-error-log-review

Details

In response to this:

/test idc-jenkins-ci-tidb/check_dev_2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

joechenrh · 2026-07-06T14:24:00Z

/test check-dev2

joechenrh · 2026-07-06T16:46:41Z

/retest

D3Hunter · 2026-07-07T02:46:16Z

/cherry-pick release-nextgen-202603

ti-chi-bot · 2026-07-07T02:47:02Z

@D3Hunter: new pull request created to branch release-nextgen-202603: #69702.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherry-pick release-nextgen-202603

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ref #69563

joechenrh added 17 commits July 6, 2026 11:47

ddl, importer: capture collation mode for background encoding

8c906d4

table: simplify collation-aware table init helpers

556f001

ddl, importer: use table collation mode in background paths

ed3adf7

ddl: avoid table collation mode in txn backfill

dec0b7b

ddl: use local collation mode in txn add index

f8a0520

ddl: keep collation snapshot for merge temp index

b9ed895

ddl: keep txn merge temp index on local collation

0fb3ffd

importer: derive encoding collation from table

02ed166

tablecodec: keep handle decode on local collation

d891385

importer: capture import collation from keyspace

390f262

table: initialize indices from table common

c50e392

table: use table collation accessor in helpers

65d4216

table: clarify table common construction

d986cd8

ddl: use reorg collation in cop context

f787d1a

ddl: simplify cop context base construction

ec4757b

ddl: simplify cop context collation constructor

3a6ac45

ddl: inline cop context index constructors

ef07673

joechenrh mentioned this pull request Jul 6, 2026

ddl, importer: use captured collation mode for background encoding #69592

Closed

ti-chi-bot Bot added the sig/planner SIG: Planner label Jul 6, 2026

ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 6, 2026

planner: keep simple expression collation local

a755f2d

coderabbitai Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread pkg/ddl/backfilling_operators.go

ti-chi-bot Bot removed do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked labels Jul 6, 2026

joechenrh and others added 4 commits July 6, 2026 17:48

tests: assert ADD INDEX collation state directly

94bf00f

tests: assert IMPORT collation state directly

69efbf0

Update pkg/meta/model/reorg.go

7e9637b

Co-authored-by: D3Hunter <jujj603@gmail.com>

model: align collation metadata test with API contract

f95c567

D3Hunter reviewed Jul 6, 2026

View reviewed changes

Comment thread pkg/executor/importer/import.go Outdated

Comment thread pkg/ddl/executor.go

*: keep collation metadata setters internal

2357d7a

D3Hunter approved these changes Jul 6, 2026

View reviewed changes

ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 6, 2026

GMHDBJD approved these changes Jul 6, 2026

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 6, 2026

ingress-bot reviewed Jul 6, 2026

View reviewed changes

tests: cover DXF collation snapshot on user keyspace

a739efc

windtalker approved these changes Jul 6, 2026

View reviewed changes

ti-chi-bot Bot added the approved label Jul 6, 2026

joechenrh added 2 commits July 6, 2026 21:08

tests: isolate DXF collation keyspace

38623f2

tests: avoid racy import collation assertion

51bbaa9

ti-chi-bot Bot merged commit 09d2140 into pingcap:master Jul 6, 2026
37 checks passed

ti-chi-bot mentioned this pull request Jul 7, 2026

*: use user keyspace's collation mode for DXF tasks (#69677) #69702

Merged

13 tasks

ti-chi-bot Bot pushed a commit that referenced this pull request Jul 7, 2026

*: use user keyspace's collation mode for DXF tasks (#69677) (#69702)

f55c185

ref #69563

Uh oh!

Conversation

joechenrh commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Clustered VARCHAR PK + secondary VARCHAR index

Clustered VARCHAR PK + secondary INT index

Composite clustered PK with VARCHAR part + secondary INT index

Composite clustered INT PK + secondary VARCHAR index

Composite CHAR PK + secondary VARCHAR index

Clustered VARCHAR PK + prefix secondary VARCHAR index

Clustered VARCHAR PK + secondary VARCHAR index + extra payload

Generated columns with string transformations

Assignment expressions with string transformations

Controls without mismatched string collation in encoded keys

Clustered VARCHAR PK + new secondary VARCHAR index

Composite clustered PK with VARCHAR part + new secondary INT index

Generated columns with string transformations

Expression indexes with string transformations

Release note

Uh oh!

coderabbitai Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

D3Hunter commented Jul 6, 2026

Uh oh!

joechenrh commented Jul 6, 2026

Uh oh!

ti-chi-bot Bot commented Jul 6, 2026

[LGTM Timeline notifier]

Uh oh!

ingress-bot left a comment

Choose a reason for hiding this comment

Summary

⚠️ [Major] (3)

🟡 [Minor] (3)

ℹ️ [Info] (1)

🧹 [Nit] (1)

Unanchored findings

⚠️ [Major] (3)

🟡 [Minor] (2)

Uh oh!

ingress-bot Jul 6, 2026

Choose a reason for hiding this comment

🟡 [Minor] New useNewCollate flag is positioned inconsistently across sibling function signatures

Uh oh!

ingress-bot Jul 6, 2026

Choose a reason for hiding this comment

ℹ️ [Info] NewCopContextBase doc comment wasn't updated to mention the new collation parameter, unlike its three sibling constructors

Uh oh!

ingress-bot Jul 6, 2026

Choose a reason for hiding this comment

🧹 [Nit] Identical admin-check-and-verify test helper duplicated across two realtikvtest packages

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot Bot commented Jul 6, 2026

Uh oh!

joechenrh commented Jul 6, 2026

Uh oh!

ti-chi-bot Bot commented Jul 6, 2026

Uh oh!

joechenrh commented Jul 6, 2026

Uh oh!

joechenrh commented Jul 6, 2026

Uh oh!

Uh oh!

joechenrh commented Jul 6, 2026 •

edited

Loading

coderabbitai Bot commented Jul 6, 2026 •

edited

Loading

ℹ️ [Info] `NewCopContextBase` doc comment wasn't updated to mention the new collation parameter, unlike its three sibling constructors